fix: use aws eks get-token for kubectl auth by schollii · Pull Request #1590 · terraform-aws-modules/terraform-aws-eks

schollii · 2021-09-17T17:10:07Z

PR o'clock

Description

Fixes #957

Use aws eks get-token instead of aws iam authenticator

Checklist

[n/a ] README.md has been updated after any changes to variables and outputs. See https://github.com/terraform-aws-modules/terraform-aws-eks/#doc-generation

lisfo4ka · 2021-10-12T14:44:03Z

  description = "Override the default name used for items kubeconfig."
  type        = string
-  default     = ""
+  default     = null


Could you clarify, please, why is it required?

probably because:

> coalescelist([""], ["c", "d"]) [ "", ] >

from line

terraform-aws-eks/locals.tf

Line 179 in 7785562

aws_authenticator_command_args = coalescelist(var.kubeconfig_aws_authenticator_command_args, local.default_kubeconfig_aws_auth_args)

actually coalesce works with both "" and null, BUT it is almost always preferable to use null than empty string to indicate "no value set".

actually coalesce works with both "" and null

yes but it will give different results (from terraform console):

> coalescelist([""], ["c", "d"]) [ "", ] > coalescelist([], ["c", "d"]) [ "c", "d", ]

ahh, but here is just normal string

yes the function being discussed is coalesce not coalescelist

lisfo4ka · 2021-10-12T14:45:53Z

@@ -219,7 +219,7 @@ variable "kubeconfig_aws_authenticator_env_variables" {
 variable "kubeconfig_name" {


would you update kubeconfig_aws_authenticator_command default value as well?

…or clarity

lisfo4ka · 2021-10-18T15:01:40Z

@antonbabenko @daroga0002 @schollii hi folks)
From my side, this change looks really good and useful. I suppose it shouldn't trigger more changes as kubeconfig file recreation which shouldn't be critical since terraform folder isn't the place to store it.

@schollii could you post terraform plan output, please. And if everyone is fine with it, let's merge this MR.

schollii · 2021-10-19T17:23:37Z

@schollii could you post terraform plan output, please...

@lisfo4ka, in what folder should I run this?

lisfo4ka · 2021-10-26T13:29:56Z

@lisfo4ka, in what folder should I run this?

I assume it can be any, e.g. examples/launch_templates

antonbabenko · 2021-10-26T17:27:58Z

+  description = "Command to use to fetch AWS EKS credentials. Defaults to aws CLI (will use eks command)."
  type        = string
-  default     = "aws-iam-authenticator"
+  default     = null


Suggested change

default = null

default = "aws"

And remove coalesce() where this variable is used. Right?

@schollii do you will be able to change it and then I will make testing

schollii · 2021-10-27T18:48:34Z

@lisfo4ka ok will report back asap

timblaktu

Changes look good to me. This will be a big improvement. All external examples of first steps configuring kubectl to auth/connect to an EKS cluster use the aws eks update-kubeconfig approach, which uses the get-token approach, so aligning this Terraform module will prevent new users from being confused why they're so different (and prevent them from having to install aws-iam-authenticator.)

bryantbiggs · 2021-11-06T19:52:30Z

I think we should include this in a breaking change and drop all support for the legacy iam-authenticator approach - thoughts @daroga0002 ?

daroga0002 · 2021-11-07T16:57:27Z

I tried to search what are benefits of using aws-iam-authenticator and I dont see any, maybe somebody from community will be able to put some arguments here.

From my side I think we can:

drop totally support for aws-iam-authenticator and remove any customization in our code
provide in outputs (I believ most or all are arleady done) values which can be used to build any form of Kube config (so thne if somebody will have own requirements can pass a template and fill them by outputs)

schollii · 2021-11-07T17:52:11Z

It's a good idea but I think it's taking this ticket beyond original scope, a separate ticket would be better so we can close this asap.

Yasumoto · 2021-11-08T00:57:23Z

Getting this over the finish line would be rad. When I was first getting started, trying to find this extra tool (and figure out if aws-iam-authenticator was actively recommended) was a bit of a road bump.

daroga0002 · 2021-11-08T11:52:45Z

It's a good idea but I think it's taking this ticket beyond original scope, a separate ticket would be better so we can close this asap.

@schollii can you adjust comments #1590 (comment) and then we will merge it to v17.* version

Work about redesign we will cover in other PR for v18* milestone

schollii · 2021-11-08T13:15:58Z

@daroga0002 not sure what you mean, clicking that link in your post just gets me to the top of the PR

daroga0002 · 2021-11-08T13:24:34Z

open comments from antonbabenko

antonbabenko · 2021-11-08T13:34:16Z

@daroga0002 I think we should have this one merged (after outstanding code changes got merged) and release a minor release in v17.

I am not knowledgeable enough to suggest dropping part of the functionality since it already works. I think we can reevaluate it as a part of the breaking changes we are going to have in v18 but not in this PR.

daroga0002 · 2021-11-08T13:41:21Z

I am not knowledgeable enough to suggest dropping part of the functionality since it already works. I think we can reevaluate it as a part of the breaking changes we are going to have in v18 but not in this PR.

Yes this is what I am suggesting, now we simply extend to allow feature in v17 and in v18 we will drop legacy approach (as that will be bigger breaking change release so migration will be not so fast probably).

I will duplicate this PR and make fixes on my branch, test and open PR as I cannot edit this.

lisfo4ka · 2021-11-23T09:28:32Z

Folks, I'd really like to deliver this change to the v17, so I've created a new PR with proper testing. Please, take a look at #1699

github-actions · 2021-12-24T00:41:41Z

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

github-actions · 2022-01-03T00:42:35Z

This PR was automatically closed because of stale in 10 days

antonbabenko · 2022-01-05T19:58:41Z

This issue has been resolved in version 18.0.0 🎉

github-actions · 2022-11-11T02:34:07Z

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

use aws eks get-token instead of aws authenticator

7785562

schollii changed the title ~~fix: use aws eks get-token instead of aws iam authenticator (~~ fix: use aws eks get-token for kubectl auth Sep 17, 2021

schollii mentioned this pull request Sep 17, 2021

fix: #957 Use aws eks get-token instead of aws-iam-authenticator #1235

Closed

2 tasks

lisfo4ka reviewed Oct 12, 2021

View reviewed changes

Oliver Schoenborn and others added 3 commits October 12, 2021 12:31

default to null kubectl auth command

7a27249

clarify variable description and re-order auth command default args f…

5c1411b

…or clarity

Merge branch 'master' into use_aws_eks_token_instead_of_authenticator

438eacd

Merge branch 'master' into use_aws_eks_token_instead_of_authenticator

0a4a966

antonbabenko reviewed Oct 26, 2021

View reviewed changes

timblaktu approved these changes Nov 5, 2021

View reviewed changes

bryantbiggs mentioned this pull request Dec 7, 2021

feat!: Removed support for launch configuration and replace count with for_each #1680

Merged

1 task

github-actions Bot added the stale label Dec 24, 2021

github-actions Bot closed this Jan 3, 2022

github-actions Bot locked as resolved and limited conversation to collaborators Nov 11, 2022

		@@ -219,7 +219,7 @@ variable "kubeconfig_aws_authenticator_env_variables" {
		variable "kubeconfig_name" {

Uh oh!

Conversation

schollii commented Sep 17, 2021

PR o'clock

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schollii Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lisfo4ka commented Oct 18, 2021

Uh oh!

schollii commented Oct 19, 2021

Uh oh!

lisfo4ka commented Oct 26, 2021 • edited by antonbabenko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

schollii commented Oct 27, 2021

Uh oh!

timblaktu left a comment

Choose a reason for hiding this comment

Uh oh!

bryantbiggs commented Nov 6, 2021

Uh oh!

daroga0002 commented Nov 7, 2021

Uh oh!

schollii commented Nov 7, 2021

Uh oh!

Yasumoto commented Nov 8, 2021

Uh oh!

daroga0002 commented Nov 8, 2021

Uh oh!

schollii commented Nov 8, 2021

Uh oh!

daroga0002 commented Nov 8, 2021

Uh oh!

antonbabenko commented Nov 8, 2021

Uh oh!

daroga0002 commented Nov 8, 2021

Uh oh!

lisfo4ka commented Nov 23, 2021

Uh oh!

github-actions Bot commented Dec 24, 2021

Uh oh!

github-actions Bot commented Jan 3, 2022

Uh oh!

antonbabenko commented Jan 5, 2022

Uh oh!

github-actions Bot commented Nov 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

schollii Oct 12, 2021 •

edited

Loading

lisfo4ka commented Oct 26, 2021 •

edited by antonbabenko

Loading